Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ekump/upgrade to rubocop 1.50.0 #3147

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

ekump
Copy link
Contributor

@ekump ekump commented Sep 20, 2023

What does this PR do?
Upgrades Rubocop from 1.34.0 to 1.50.0

Motivation:
While working on another branch, I encountered a bug where attempting to disable Lint/MissingSuper led to a Lint/RedundantCopDisableDirective error that could not be suppressed.

Additional Notes:
Uncovered a test in profiling/integration_spec.rb that may have been running as expected.

How to test the change?
bundle exec rake rubocop

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@ekump ekump requested a review from a team September 20, 2023 22:15
@github-actions github-actions bot added core Involves Datadog core libraries integrations Involves tracing integrations tracing labels Sep 20, 2023
@ekump ekump force-pushed the ekump/upgrade-to-rubocop-1-50-0 branch from b67f109 to 9e9ea05 Compare September 20, 2023 22:16
@@ -160,11 +160,11 @@ def obfuscate_query(query, options = {})
(?:"|%22) # closing '"' at end of value
)
|(?: # other common secret values
bearer(?:\s|%20)+[a-z0-9._\-]+
bearer(?:\s|%20)+[a-z0-9._-]+
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex changes are just removing redundant escape characters

@@ -255,9 +255,8 @@ def stack_frame_to_function_id(backtrace_location)
is_expected.to have(4).items

# All but last are unique
(0..-2).each do |i|
3.times do |i|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivoanjo wanted to bring this to your attention. (0..-2).each never executes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, thanks for spotting it! The good news is -- this test is only for the old profiler codepath, that is off by default and no customers should be using anymore.

The new profiler has similar specs, but in different files, and doesn't have this bug as far as I can spot it.

(The old profiler, along with this file are going to be deleted in Q4; I've mostly been finishing up a few other features before I come and remove all of this.)

@ekump ekump force-pushed the ekump/upgrade-to-rubocop-1-50-0 branch 4 times, most recently from 27acf78 to 7c29267 Compare September 21, 2023 14:49
@ekump ekump force-pushed the ekump/upgrade-to-rubocop-1-50-0 branch from 7c29267 to fd676ec Compare September 21, 2023 15:03
@ekump ekump changed the title ekump/upgrade to rubocop 1 50 0 ekump/upgrade to rubocop 1.50.0 Sep 21, 2023
RUBY_ENGINE == 'ruby'.freeze
'ruby'.freeze == RUBY_ENGINE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug of the https://www.rubydoc.info/github/bbatsov/RuboCop/RuboCop/Cop/Style/YodaCondition cop.
And I believe it happens because it doesn't know that #freeze is not really a complex method call, and 'ruby'.freeze is still an immediate value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this file, can I suggest adding # frozen_string_literal: true to the top, and reverting the YodaCondition changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit.

@ekump ekump requested review from a team as code owners September 25, 2023 15:36
@ekump ekump merged commit a6024a7 into master Sep 25, 2023
88 of 103 checks passed
@ekump ekump deleted the ekump/upgrade-to-rubocop-1-50-0 branch September 25, 2023 15:49
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants